Skip to content

Conversation

@theblackunknown
Copy link
Contributor

@theblackunknown theblackunknown commented Aug 5, 2024

Description of Change(s)

Using CMake namespace target is the modern approach, it forwards definitions, include directories and link libraries transitively to consumers.
One TBB::tbb is already produced by the FindTBB.cmake module.
As the main CMakeLists.txt already requires CMake 3.14 this should already be available to all CMake clients.

Additionally try to resolve TBB Config package before trying the module one.
Miscellaneous associated cleanups are made with remaining commits.

  • I have verified that all unit tests pass with the proposed changes
  • I have submitted a signed Contributor License Agreement

@theblackunknown theblackunknown marked this pull request as ready for review August 5, 2024 08:16
@jesschimein
Copy link
Collaborator

Filed as internal issue #USD-9934

@jesschimein
Copy link
Collaborator

/AzurePipelines run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@theblackunknown theblackunknown force-pushed the cmake-find-tbb branch 2 times, most recently from d17762c to a1075a6 Compare August 6, 2024 14:44
@theblackunknown
Copy link
Contributor Author

theblackunknown commented Aug 6, 2024

Hey @jesschimein I have reproduced yesterday issues locally when building without using oneTBB and I have pushed a new fix to accommodate those.

@jesschimein
Copy link
Collaborator

/AzurePipelines run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@theblackunknown
Copy link
Contributor Author

Hi @jesschimein , I am not sure to understand whether the last azure pipeline run for windows is a genuine error or not.
I am not able to reproduce the timeout issue locally.

@jesschimein
Copy link
Collaborator

/AzurePipelines run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@jesschimein
Copy link
Collaborator

Yeah, some of the timeouts are a little fickle, and we usually rerun to check. Looks like Windows successfully built this time! 🙌🏻

@theblackunknown
Copy link
Contributor Author

Hi @jesschimein there is now a large list of conflicts, if I resolve them is there a good chance to integrate my change for the next release ?

@asluk asluk added the build Build-related issue/PR label Oct 25, 2024
@spiffmon
Copy link
Member

spiffmon commented Mar 1, 2025

Hi @theblackunknown , we would like to consider this for one of the shortly upcoming releases, so yes please, could you resolve the conflicts (which came about from our added support for OneTBB, I think), and also ensure the changes work with OneTBB as well as a stock build_usd.py build?

Thank you!

@theblackunknown
Copy link
Contributor Author

Hi @theblackunknown , we would like to consider this for one of the shortly upcoming releases, so yes please, could you resolve the conflicts (which came about from our added support for OneTBB, I think), and also ensure the changes work with OneTBB as well as a stock build_usd.py build?

Thank you!

Sure thing ! I'll allocate time in the following two weeks to rebase my work and test my changes.

@theblackunknown theblackunknown force-pushed the cmake-find-tbb branch 2 times, most recently from 812516f to 62521c4 Compare March 9, 2025 20:57
@jesschimein
Copy link
Collaborator

/AzurePipelines run

@azure-pipelines
Copy link

Azure Pipelines could not run because the pipeline triggers exclude this branch/path.

@theblackunknown
Copy link
Contributor Author

@spiffmon I have rebased and did confirm that my changes work with build_usd.py both with and without --onetbb.

That said, it looks like I need to rebase one more time my changes.
I'll schedule time to do so this week.

@jesschimein
Copy link
Collaborator

/AzurePipelines run

@azure-pipelines
Copy link

Azure Pipelines could not run because the pipeline triggers exclude this branch/path.

@theblackunknown
Copy link
Contributor Author

@jesschimein @spiffmon I believe this PR is now ready to do, is this schedule on your internal queue for some later release ?

@spiffmon
Copy link
Member

Yes, @theblackunknown ! There's not enough time in the 25.05 release that we are wrapping up this week, but we have it queued for the release after that - thank you!

@jesschimein
Copy link
Collaborator

/AzurePipelines run

…argets

Using CMake namespace target is the modern approach, it forward definitions, include directories and link libraries transitively to consumers.
And one is already setup by the `FindTBB.cmake` module.
As the main CMakeLists.txt already requires CMake 3.14 this should already be available to all CMake clients.

- Additionally try to resolve TBB Config package before trying the module one.

- Add missing linkage to TBB to targets which use it directly

- Remove extra linkage to TBB to targets which dot not use it directly

- Add missing call to `find_dependency(TBB ..)` to resolve indirect dependencies

- Fix typo in FindTBB

- Implement heuristic to resolve import/shared libraries on Windows
@jesschimein
Copy link
Collaborator

/AzurePipelines run

@matthewcpp
Copy link
Contributor

Thank you for submitting this fix! I did have to make one small additional modification to FindTBB.cmake in order to fix a linker issue with the linux build. Adding the INTERFACE_LINK_DIRECTORIES to the tbb targets made everything compile successfully.

We are planning to include this fix in the 25.08 release.

@pixar-oss pixar-oss closed this in 0ca6d5f Jun 20, 2025
meshula pushed a commit to meshula/USD that referenced this pull request Sep 11, 2025
Adds additional targets for TBB components.
Updates various CMakeLists.txt files to use TBB::tbb component
Removes explicit TBB dependencies from CMakeLists.txt files for modules for which it is not necessary
Updates pch headers for modules which do not utilize TBB

Closes PixarAnimationStudios#3207

(Internal change: 2369739)
(Internal change: 2369797)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

build Build-related issue/PR

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants